Additional SNMP library output formatting options - #21502
Conversation
0a5be4a to
443eb75
Compare
443eb75 to
1de112d
Compare
|
The net-snmp library has quite a few options for controlling how OIDs and values are displayed. PHP already has a number of examples of these: The base purpose of this PR is to extend the number of options available to include all options supported by the net-snmp library. The SNMP library configuration is stored in global memory, so some of the code takes care of handling the state of the library when using the PHP SNMP object. The implementation is to add 3 new functions that control all options for MIB parsing, string output and value output. These do duplicate the existing functions like snmp_set_enum_print(), snmp_set_quick_print(), but having both doesn't cause any issues. I think the new interface is more flexible (we only need a new ENUM for each new options instead of a get/set), and there is no harm in having both functions available for existing options. There is an option to deprecate the old functions if you want to standardise the way people control the library (and also to add get functions to read the current state of a given option). I also added all new options as parameters to the PHP SNMP object. Whenever a SNMP query is run from a PHP SNMP object, the existing state of the net-snmp library needs to be saved, then updated to match what the SNMP object wants, and finally restored back to the original state. I have created 2 functions (save_snmplib_output_options() and set_snmplib_output_options()) to make it easier to save and restore the state. I also discovered that changes to the SNMP library state were persisting across PHP-FPM requests. I did this by running a script that updated the state, then running another script many times that performed a SNMP query and printed the output. The query script would produce inconsistent output depending on which FPM process handles the request. This led me to add PHP_RINIT_FUNCTION() and PHP_RSHUTDOWN_FUNCTION() code into the SNMP module to save the state of the SNMP library before a FPM request, and then restore it at the end. |
| /** | ||
| * @var int | ||
| * @cvalue NETSNMP_STRING_OUTPUT_GUESS | ||
| */ | ||
| const SNMP_STRING_OUTPUT_GUESS = UNKNOWN; | ||
| /** | ||
| * @var int | ||
| * @cvalue NETSNMP_STRING_OUTPUT_ASCII | ||
| */ | ||
| const SNMP_STRING_OUTPUT_ASCII = UNKNOWN; | ||
| /** | ||
| * @var int | ||
| * @cvalue NETSNMP_STRING_OUTPUT_HEX | ||
| */ | ||
| const SNMP_STRING_OUTPUT_HEX = UNKNOWN; |
There was a problem hiding this comment.
Can these be combined? If not maybe an enum is better so there is type safety.
There was a problem hiding this comment.
What do you mean by type safety? I've extended the original technique used in the php-snmp code to copy the NETSNMP* enums directly so I can use pass them through to the netsnmp library calls. All values are checked before being used.
There was a problem hiding this comment.
What I mean was to use a PHP enum that translates to the C values (by using and use Z_PARAM_ENUM), rather than passing an int. It's type safety for the end user writing PHP code.
But if those contants can be bitmasked, that technique doesn't work.
There was a problem hiding this comment.
How does this look now?
The only issue I can see is that the proposal was to merge this into the next release for all current versions, but Z_PARAM_ENUM() is not implemented in PHP 8.4 (and 8.5?)
There was a problem hiding this comment.
We don't add new features to stable releases in patch versions according to our policy, see https://github.com/php/policies/blob/main/release-process.rst#patch-version-number.
Nobody caught this during discussion or voting as this is not something that is usually changed.
I'll send an email to internals to clarify this and also the enum change.
There was a problem hiding this comment.
That's fine - I can see why using enums is good, and can't see how they could be easily used for existing releases, and agree there's no point in rolling out new functions with constants when we know we want to move to enums.
I'll just need to wait for the next release before I can make use of the new functions.
a23a795 to
a468bbf
Compare
79547a6 to
fe59e60
Compare
|
I have changed the stub to reference the enum for all functions, and converted all the appropriate default: cases to use ZEND_UNREACHABLE(). I also rebased against the new master branch following the merge of the mib reset PR. Let me know if there's any more changes needed. Maybe some tests? |
4eb5617 to
9feefe6
Compare
Girgias
left a comment
There was a problem hiding this comment.
Hope you didn't mind me squashing and rebasing this after I split some of the other changes to another PR.
The only thing I'm a bit weary about is changing the snmp_set_oid_output_format and adding the new corresponding method to the class as this interferes with the property of the same name.
|
If you can add more tests that would be great, but we already have issues with flaky SNMP tests that might be also good to fix in a different PR. |
9feefe6 to
cc56f57
Compare
No issues with the squash and rebase - I was aware that the namespace stuff and changing the return types was extending the scope of what this PR, and adding noise to the diffs. The diff for this PR looks much better now. I've changed the error message for snmp_set_oid_output_format() to encourage the use of the enum, but the code still accepts an integer with the SNMP_OID_OUTPUT_* constants. The constants themselves have changed to mirror the enum values, so existing PHP code will still work as long as the end users have followed the documentation and used the constant names. In fact, as it stands today they map to the same values, so even if someone has used the number 1 instead of the constant name it will still work. The same thing goes for the new method on the class - changes using the method will still be reflected in the integer property, and end users can still update using the PHP constants. Thinking about this further I have updated the code for reading the oid_output_format property of the class so it maps from the ENUM value back to the CONST. Does the above sound correct (I haven't changed the existing behaviour - I have just extended existing functions to allow the enum or const), or is there something else I'm missing? |
RFC: https://wiki.php.net/rfc/snmp_improvements_2026
The SNMP library has more formatting options than php-snmp allows. This PR adds some additional functions to control how the net-snmp library returns results to PHP.